Add optimise timeline feature #2180
Conversation
There was a problem hiding this comment.
LGTM! Just one nitpick:
The "state" of the button doesn't persist across refreshes. The "breakdown by file type" and "merge all groups" checkboxes retain their state when refreshed, so I think it would be good for the new checkbox to follow this behaviour as well!
|
@sopa301 thank you and fixed! |
There was a problem hiding this comment.
Great start @jonasongg! The optimised timelines are looking good and would definitely improve the usability of RepoSense!
While most of the changes you've made look good, there are some things left out and some design decisions to discuss too.
- [High priority] The chart zoom feature (selected using Ctrl/Cmd + click) does not work correctly when the timelines are optimised, i.e., the dates used for the zooming in are the same as the original dates instead of the optimised dates, which is misleading to the user.
- [Low priority] When I open the Commits Panel for an optimised chart, the chart shown at the top of the Commits Panel is not optimised.
- [Discuss] When the report is grouped by repos, the various authors under the repo may have different timelines. This discrepancy may make it difficult to compare when different teammates are contributing relatively. We may want to make the timeline duration consistent across a repo group (@damithc). However, when grouping by authors, this doesn't matter.
- [Discuss] We may want to think of a more intuitive name for "Optimise timeline", because it is not immediately clear what this feature does to end users. I would suggest something along the lines of "Condense charts".
|
Example repo before and after the optimise timelines option is activated. Before: After: |
|
Good inputs @ckcherry23
One option (probably easier too) is to take the stance that 'optimized' timelines isn't suitable for timeline comparisons. What do you all think?
'Trim'? |
|
@ckcherry23 @damithc thank you for your comments! will do the necessary changes asap. as for when we are grouping by repos, it shouldn't be too difficult to make the timeline consistent across a group (i think we can just select the minimum and maximum commit date of the group instead of the user). i'm ok to implement either option! i think "trim timeline" works! |
There was a problem hiding this comment.
Awesome work @jonasongg! The ramps and the chart zoom work well now with the trim timelines feature!
However, fixing the zoom feature broke the optimization of the default ramp chart shown on the Commits Panel when "trim timelines" is enabled, sorry for the oversight. Maybe the best option is to pass the optimized timeline min/max dates conditionally along with filterSinceDate and filterUntilDate for this in lines 103 and 221 of c-summary-charts.vue?
The rest of the changes look good to me. As discussed, since we need other collaborators' help for adding Cypress tests, I think we can ignore that comment and ask for our second reviewer feedback now.
…line' into add-optimise-timeline
There was a problem hiding this comment.
LGTM! Thanks for this @jonasongg! This would be a super useful feature for setting up personal RepoSense reports!
|
@MarcusTXK the tests were actually added in this PR under optimiseTimeline.cy.js - should I change this? |
|
@jonasongg Ah OK, apologies I missed it. No worries, this works, let's leave it as it is then. |
|
The following links are for previewing this pull request:
|
Fixes #2162
Part of #2157
Proposed commit message
Other information